-
Notifications
You must be signed in to change notification settings - Fork 603
Use dialects in the parser for support snowflake aliasing syntax #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pull Request Test Coverage Report for Build 189751011
💛 - Coveralls |
43c61af
to
a4787f1
Compare
464b770
to
2b26ac5
Compare
…syntax Snowflake DB allow single table to be within parenthesis. This behaviour is diffrent than other DB , and it has some impact on the parsing table factor. For supporting we do the following : 1. Add refrence to the dialect in the parser 2. Add Snowflake dialect 3. add function to the dialect trait the identify if single table inside parenthesis allowed 4. When parsing table factor in the allow/deny single table inside parenthesis according to dialect
…lias)` and update the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit with an alternative implementation of your idea: 94919ba - what do you think?
Also see some comments inline:
fn alllow_single_table_in_parenthesis(&self) -> bool { | ||
false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note #241 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry :| - I don't understand what you mean by that ..
(to note #241 in code comment ? or something in the code is not aligned with what you commented in #241 (comment) )
Can you please elborate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that comment I tried to argue for using if self.dialect <is snowflake>
checks rather than self.dialect.alllow_single_table_in_parenthesis()
, at least until we better understand when to use which approach.
#[derive(Debug, Default)] | ||
pub struct SnowflakeDialect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like the new dialect (including a separate test file), the dialect-specific parsing infrastructure, and the table factor parsing fix to be landed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean by different PR's or commits ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant PRs.
macro_rules! nest { | ||
($base:expr $(, $join:expr)*) => { | ||
TableFactor::NestedJoin(Box::new(TableWithJoins { | ||
relation: $base, | ||
joins: vec![$(join($join)),*] | ||
})) | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't duplicate code.
fn sf() -> TestedDialects { | ||
TestedDialects { | ||
dialects: vec![Box::new(SnowflakeDialect {})], | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please place this at the end of the file, and include a sf_and_generic()
helper, as in other files.
} | ||
} | ||
|
||
fn get_from_section_from_select_query(query: &str) -> Vec<TableWithJoins> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been able to carefully review the rest, because the move of existing tests and the addition of new ones are combined in a single commit (see the request above).
Thanks for your comment and for the commit with the suggestions (i also tech me some features or rust that i was not aware of ) , agree thats more elegant - and i will merge it into the PR . I also asked inline for some clarifications for some off the comments , as i don't sure what the action item for them .. |
Ok, so i am closing this PR - and will open 3 smaller.. |
Snowflake DB allow single table to be within parenthesis.
This behaviour is diffrent than other DB ,
and it has some impact on the parsing table factor.
For supporting we do the following :